-
-
Notifications
You must be signed in to change notification settings - Fork 406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: add new timing capabilities to modifier manager #883
base: master
Are you sure you want to change the base?
Conversation
Flash of unstyled content
Seems good. 👍🏼 I think implementing support for this in import { modifier, Modifier } from 'ember-modifier/name-TBD'; We need to think about how to name that, of course, but you can see how that approach works nicely from a design POV: it means the default is to use the existing semantics (which is what most users should use) while the more costly, but sometimes useful, semantics. That said, I think it will be important to think about the relationship between the other possible timing semantics as well for the evolution of that library: before-insertion seems like it can just be another timing mode users will be able to opt into, but template-invocation-ordering is an orthogonal condition which seems likely to be cross-cutting, and it isn't obvious how to guarantee it given existing semantics about when and how we do re-renders—trying to enforce that guarantee isn't particularly natural in the context of Glimmer templates today. |
Yes. That's the idea. The most challenging part for that is to not complicate server-side rendering support even more. For server-side rendering a
In the discussion I had with @wycats we tend to not support it. So far grabbing a reference to an element to be used later e.g. with |
While this might be a good interim solution, I don't see it as a great high-level API for long-term. It limits modifiers to schedule work for either of the timings. But does not expose the power of scheduling some work for on layout and some other work for on idle. An event-driven API may fit better with added flexibility. It might look like this: modifier.on('layout', (element) => {
// do some work, which needs to be done before browser paints the element
});
modifier.on('idle", (element) => {
// do some other work, which could not cause a flash of unstyled content issue
});
modifier.on('idle', () => {
// could split up the work in logically independent chunks to structure the code
});
modifier.on('destroy', () => {
// teardown logic
}); But deciding on a high-level API is out of scope for this RFC. Thanks to low-level primitives provided by modifier manager, we can experiment with those and gather experience before committing to a specific high-level API. |
The text needs some clarification, then: as it stands the two capabilities/hooks appear to be installation-specific and that strongly implies that only one of them runs. (The text doesn’t explicitly say that but it’s what I inferred from the name!) If both hooks run, then I agree that we’d want to approach it differently; but we should update the hook names to be clear about that. |
coming from Starbeam, and having worked with resources a little bit, I think I'd prefer a unified API for all things ™ for example (and with the caveat that this is very hand-wavy (except for the resource example, which comes from the docs)): resources: import { Cell, Resource } from "@starbeam/universal";
const Stopwatch = Resource(({ on }) => {
const start = Cell(new Date());
const interval = setInterval(() => {
start.set(new Date());
}, 1000);
on.cleanup(() => {
clearInterval(interval);
});
return start;
}) and an eventual modifier would be a resource that takes the element as an argument: import { Modifier } from 'somewhere';
const Style = Modifier(({ element, on }) =>{
// what's the timing in the body space here? pre-insert?
on.layout(() => { /* set the style attribute */ });
on.idle(() => { /* wiggle using the WebAnimation API */ });
on.cleanup(() => { /* remove changes */ });
}); While this looks about the same, if you squint, I think it provides better ergonomics for editors and language servers. But this is maybe getting off topic for what this RFC is intending to introduce. I'm overall 💯 in favor of introducing pre-paint timings -- it would improve the perf of a lot of things. |
I agree. The current naming is misleading. Having more than one hook, which is executed on first render does not fit well in the existing lifecycle model for modifiers in general. It currently has the following:
Maybe we need to have something like this instead:
Maybe we want to provide a reference to the update lifecycle hook in that case as well. It would not fit well in the mental model if a modifier must implement one of the setup hooks just to grab a reference to the element. |
I renamed the hooks to |
how has exploration been going? This is highly needed! <3 |
@NullVoxPopuli I think part of our challenge here is making sure that we're actually going to do the work. Is this something you'd be interested in helping with? |
yes, It is -- partially to help with graphics and chart rendering performance, and partially to help out with ergonomics and performance in how ember-headless-table is wired |
Another nice use case for this: merging tailwind classes for component library use cases: https://twitter.com/buschtoens/status/1658483457000103938 |
@wycats Are there any capabilities in things like Starbeam that impact this RFC? Do you have any context here? |
Rendered